Range mappings: spec text for proposal #233
Conversation
Adds a flag to Decoded Mapping Records which indicate if a mapping is a range mapping. Populate this field based on the decoded range mapping offsets.
| 1. If the result of performing ComparePositions(_last_.[[GeneratedPosition]], _mapping_.[[GeneratedPosition]]) is ~equal~, then | ||
| 1. Append _mapping_.[[OriginalPosition]] to _originalPositions_. | ||
| 1. <ins>If _mapping_.[[IsRangeMapping]] is true, then</ins> |
There was a problem hiding this comment.
(copy-pasting my message from the other PR, to make it more visible)
If we are looking at the mappings at e here:
abcdefg
and we have none but we have two mappings right before at c, out of which one is the range mappings, maybe we should exclude the non-range one? Since the range one actually ends up being an exact match.
Not exactly sure though.
| 1. If _state_.[[NameIndex]] < 0 or _state_.[[NameIndex]] ≥ the number of elements of _names_, optionally report an error. | ||
| 1. Else, set _name_ to _names_[_state_.[[NameIndex]]]. | ||
| 1. Let _decodedMapping_ be a new DecodedMappingRecord { [[GeneratedPosition]]: _generatedPosition_, [[OriginalPosition]]: _originalPosition_, [[Name]]: _name_ }. | ||
| 1. <ins>Let _isRangeMapping_ be the result of performing LookupRangeMapping with arguments _rangeMappings_, _state.[[GeneratedLine]], and _state.[[MappingIndex]].</ins> |
There was a problem hiding this comment.
| 1. <ins>Let _isRangeMapping_ be the result of performing LookupRangeMapping with arguments _rangeMappings_, _state.[[GeneratedLine]], and _state.[[MappingIndex]].</ins> | |
| 1. <ins>Let _isRangeMapping_ be the result of performing LookupRangeMapping with arguments _rangeMappings_, _state_.[[GeneratedLine]], and _state_.[[MappingIndex]].</ins> |
| 1. Let _namesField_ be GetOptionalListOfStrings(_json_, *"names"*). | ||
| 1. Let _mappings_ be DecodeMappings(_mappingsField_, _namesField_, _sources_). | ||
| 1. <ins>Let _rangeMappingsField_ be GetOptionalString(_json_, *"rangeMappings"*).</ins> | ||
| 1. <ins>If _rangeMappingsField_ is *null*, set _rangeMappingsField_ to *""*.</ins> |
There was a problem hiding this comment.
Nit: just handle this in the decode function?
| </emu-grammar> | ||
| <emu-alg> | ||
| 1. Perform DecodeRangeMappingsField of |RangeMapping| with arguments _state_ and _rangeMappings_. | ||
| 1. Perform DecodeRangeMappingsField of |RangeMappingList| with arguments _state_, _rangeMappings_. |
There was a problem hiding this comment.
| 1. Perform DecodeRangeMappingsField of |RangeMappingList| with arguments _state_, _rangeMappings_. | |
| 1. Perform DecodeRangeMappingsField of |RangeMappingList| with arguments _state_ and _rangeMappings_. |
| <td>a non-negative integer</td> | ||
| </tr> | ||
| <tr> | ||
| <td><ins>[[MappingIndex]]</ins></td> |
There was a problem hiding this comment.
Nit: can we call this something with "range mapping"?
| <emu-alg> | ||
| 1. Let _relativeIndex_ be the VLQUnsignedValue of |Vlq|. | ||
| 1. If _relativeIndex_ is 0, then | ||
| 1. Optionally report an error. |
There was a problem hiding this comment.
Does this mean the first mapping can never be a range mapping?
"mappings": "AAAA",
"rangeMappings": "A"There was a problem hiding this comment.
It can be, because the indices are 1-based and the index counter starts at 0. So specifying B gives you the first mapping in this encoding.
| 1. If _relativeIndex_ is 0, then | ||
| 1. Optionally report an error. | ||
| 1. Set _state_.[[MappingIndex]] to _state_.[[MappingIndex]] + _relativeIndex_. | ||
| 1. Let _rangeMappingOffset_ be a new Decoded Range Mapping Offset Record { [[GeneratedLine]]: _state_.[[GeneratedLine]], [[MappingIndex]]: _state_.[[MappingIndex]] - 1 }. |
There was a problem hiding this comment.
Why are we dealing with 1-based indexes?
There was a problem hiding this comment.
WIth 1-based indices the error check is pretty simpler, and it's just to reject 0. In the future we could re-use the 0 value for something if there's a need.
If it were 0-based, we would need to error or no-op if a 0 shows up but it's not the first value in the line.
| 1. Set _state_.[[GeneratedColumn]] to 0. | ||
| 1. Perform DecodeMappingsField of |LineList| with arguments _state_, _mappings_, _names_ and _sources_. | ||
| 1. <ins>Set _state_.[[MappingIndex]] to 0.</ins> | ||
| 1. Perform DecodeMappingsField of |LineList| with arguments _state_, _mappings_, _names_<ins>,</ins> <del>and</del> _sources_ <ins>and _rangeMappings_</ins>. |
There was a problem hiding this comment.
I think we should leave DecodeMappingsField alone, and instead pass mappings to DecodeRangeMappings. It allows us define error cases for
- range mapping point to a
GenColumnsourceless mapping - range mapping points to undefined mapping (line too large or index too large)
There was a problem hiding this comment.
That seems like a good idea, I'll try that out
This is the actual PR for the draft range mappings proposal for detailed review. Comments and feedback welcome.
On the other PR #232, Nic raised some points about what we should do in case there are multiple mappings in one location and only one (or a subset) are range mappings.